Skip to content

Address TODO items: safe_inference migration, module splits, np.dot, and bug fixes#163

Merged
igerber merged 1 commit intomainfrom
address-todo-items
Feb 17, 2026
Merged

Address TODO items: safe_inference migration, module splits, np.dot, and bug fixes#163
igerber merged 1 commit intomainfrom
address-todo-items

Conversation

@igerber
Copy link
Copy Markdown
Owner

@igerber igerber commented Feb 17, 2026

Summary

  • Migrate ~32 inline inference sites to safe_inference() across 11 source files for consistent NaN-safe inference
  • Split 3 oversized modules into sub-modules: trop.py → trop_results.py; imputation.py → imputation_results.py + imputation_bootstrap.py; two_stage.py → two_stage_results.py + two_stage_bootstrap.py
  • Replace @ operator with np.dot() at ~30 observation-dimension matrix sites to avoid Apple M4 BLAS spurious RuntimeWarnings
  • Fix diagnostics.py:785 SE returning 0.0 instead of NaN for undefined standard errors
  • Fix test_two_stage.py::test_nan_propagation no-op test (was pass with no assertions)
  • Add regression tests for staggered.py CI bug fix, np.dot warnings, and diagnostics SE behavior
  • Update CLAUDE.md, TODO.md, and REGISTRY.md documentation

Methodology references (required if estimator / math changes)

  • Method name(s): All estimators (inference computation consistency)
  • Paper / source link(s): N/A — no methodology changes, only inference computation consolidation
  • Any intentional deviations from the source (and why): TROP CI now uses t-distribution (was normal-distribution for CI but t-distribution for p_value — inconsistent). After safe_inference() migration, both use t-distribution. Documented in REGISTRY.md.

Validation

  • Tests added/updated: test_diagnostics.py (SE=NaN regression), test_linalg.py (np.dot no-warnings), test_staggered.py (NaN-SE CI regression), test_two_stage.py (test_nan_propagation fix)
  • Full test suite: 735+ tests passing across all affected modules (two_stage: 56, imputation: 76, trop: 78, staggered: 95, estimators: 142, diagnostics: 32, linalg: 66, sun_abraham: 47, utils: 83, triple_diff: 50)
  • Backward compatibility verified: all re-exported imports work (from diff_diff.X import Y)

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

@igerber @claude
Address TODO items: safe_inference migration, module splits, np.dot, …
089117f 
…and bug fixes

Migrate ~32 inline inference sites to safe_inference() across 11 source files,
split 3 oversized modules (trop, imputation, two_stage) into sub-modules,
replace @ operator with np.dot() at observation-dimension sites to avoid
Apple M4 BLAS spurious warnings, fix diagnostics.py SE=0.0 bug, and fix
test_two_stage.py test_nan_propagation no-op test.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 17, 2026

Overall assessment: ✅ Looks good

Executive summary:

  • Safe inference migration is consistent across the touched estimators, and NaN-gating now aligns with the library’s stated inference policy.
  • TROP’s CI distribution change (normal → t) is implemented with df = max(1, n_treated_obs - 1) and is documented in the Methodology Registry.
  • Module splits for imputation/two_stage/trop preserve re-exports and keep public API stable.
  • Added tests cover NaN propagation and regression cases for CI/SE behavior and the np.dot warning fix.
  • Only gap I see is a missing targeted test for the TROP CI distribution change.

Methodology

  • No findings. TROP CI distribution change is documented and matches code in diff_diff/trop.py:1172 and diff_diff/trop.py:1746, consistent with docs/methodology/REGISTRY.md:945.

Code Quality

  • No findings.

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • No findings.

Security

  • No findings.

Documentation/Tests

  • P3 Impact: The TROP CI distribution change (t vs normal) is not explicitly regression-tested, so a future refactor could revert to inconsistent CI/p_value without detection. Concrete fix: add a small unit test in tests/test_trop.py that computes the CI for a small n_treated_obs and asserts t-based critical values are used. Location: diff_diff/trop.py:1172 and diff_diff/trop.py:1746, registry note docs/methodology/REGISTRY.md:945.

Tests not run (not requested).

@igerber igerber merged commit 62b663b into main Feb 17, 2026
11 checks passed
@igerber igerber deleted the address-todo-items branch February 17, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@igerber